fix(skills): safely tokenize portable allowed-tools patterns - #4984
fix(skills): safely tokenize portable allowed-tools patterns#4984georgelichen wants to merge 5 commits into
Conversation
Portable Agent Skills declarations such as Bash(tvly *) contain spaces inside a command pattern. Keep those patterns as single literal entries while preserving exact names from the existing YAML-list form, so skill loading no longer fragments valid metadata or rewrites mixed-case MCP tools. Constraint: DeerFlow's current skill policy matches exact tool names and does not inspect Bash arguments Constraint: Agent Skills scalar syntax uses whitespace-separated entries with parenthesized command patterns Rejected: raw.split() | fragments Bash(tvly *) into unrelated tool names Rejected: normalize YAML-list entries | breaks case-sensitive MCP/runtime tool names Rejected: map Bash(...) to bash | broadens command-scoped declarations into unrestricted shell access Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep Bash(...) entries literal and inactive until DeerFlow has an explicit command-pattern authorization model Tested: 175 focused parser, validation, installer, review, loader, and tool-policy tests; Ruff check and format; compileall; git diff --check Not-tested: Full backend suite stopped at pre-existing Windows mode assertion test_runtime_config_store_file_is_owner_only Related: bytedance#4912
73fc086 to
316809f
Compare
github-actions
Bot
added
area:docs
|
Thanks for the update. Two issues remain:Scalar normalization may rewrite exact or custom tool names (parser.py L22–28), while runtime policy uses exact matching (tool_policy.py L54–65). The tokenizer also does not handle quoted or escaped parentheses (parser.py L37–52). |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
The parenthesis-aware direction and list-preservation change are good, but scalar alias normalization can still substitute a different runtime authority for an exact lowercase tool name. The existing discussion also notes the separate quote/escape parsing issue; I have not duplicated that inline.
| return tool_name | ||
| snake_case = _ACRONYM_BOUNDARY_RE.sub("_", tool_name) | ||
| snake_case = _CAMEL_CASE_BOUNDARY_RE.sub("_", snake_case).casefold() | ||
| return _PORTABLE_TOOL_ALIASES.get(snake_case, snake_case) |
There was a problem hiding this comment.
[P1] Do not map exact lowercase custom tools onto different built-ins. Because the alias lookup happens after casefold(), a scalar declaration such as allowed-tools: write is converted to write_file. The runtime policy then removes an installed tool actually named write and exposes DeerFlow's real file-writing tool instead. This is an authority substitution, not only a failed exact match. Please restrict Read/Write/Edit aliases to the intended portable spellings before case folding, or resolve normalization without granting a second tool when the literal name is an exact runtime tool. Add a policy regression case with both write and write_file present.
There was a problem hiding this comment.
Addressed in commit 7b62d0f.
The portable alias table now matches exact documented spellings only; alias lookup no longer case-folds arbitrary scalar names. Therefore Write maps to write_file, while a literal write remains write. Added an activation-level regression with both write and write_file present to verify that the declared authority is not substituted.
| skill_dir.mkdir() | ||
| skill_file = skill_dir / "SKILL.md" | ||
| skill_file.write_text( | ||
| "---\nname: portable\ndescription: Portable tools\nallowed-tools: WebFetch Bash(git:*)\n---\nBody\n", |
There was a problem hiding this comment.
[Suggestion] Exercise the new spaced-pattern path at activation level. Bash(git:*) does not contain internal whitespace, so this policy test would also pass with the old raw.split() implementation. Consider using a declaration such as Bash(git add *) and including a tool named add in the request; that pins the security-relevant contract that command fragments cannot accidentally become allowed business tools.
There was a problem hiding this comment.
Updated the activation regression to use Bash(git add *) and include a tool named add in the model request. The policy keeps the parenthesized declaration as one inactive literal token, so both bash and the add fragment are rejected while the explicitly normalized web_fetch tool remains available.
Portable scalar frontmatter needs alias normalization for known DeerFlow-compatible names, but generic case conversion corrupts MCP and custom tool identifiers. The tokenizer also treated quoted or escaped parentheses as structural delimiters, rejecting valid command patterns. Preserve unknown names and parse quoted or escaped patterns without broadening Bash(...) into bash. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Parenthesized patterns remain literal because argument-level authorization is not implemented Rejected: Generic CamelCase-to-snake_case for every scalar | rewrites custom/MCP names Rejected: Map Bash(...) to bash | broadens command-scoped declarations into unrestricted shell access Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add an explicit alias before supporting another portable tool name; keep command-pattern authorization separate Tested: 225 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite remains affected by unrelated Windows permissions/path and missing Lark CLI tests Related: bytedance#4984; bytedance#4912
|
Regarding comment #5406980002, the two parser concerns have been addressed in commit 3979b313.
Added parser and activation-level regression tests for exact MCP/custom names and quoted/escaped parenthesized patterns. Focused skills validation: 225 passed, 1 skipped. Ruff check, Ruff format check, compileall, and git diff --check also pass. The change remains scoped to portable frontmatter parsing and does not add unrelated integration behavior. |
Case-folding a scalar declaration before alias lookup can turn literal write into write_file, substituting a different runtime authority. Keep exact portable spellings as aliases and preserve lowercase, custom, and MCP names; strengthen activation coverage for spaced Bash patterns and command fragments. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Bash(...) remains literal and inactive because command-pattern authorization is not implemented Rejected: Case-insensitive alias lookup | maps lowercase runtime tools onto built-in authorities Rejected: Broaden the parser into command-pattern authorization | outside this PR's scope Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add aliases only for documented portable spellings; preserve all other scalar names verbatim Tested: 226 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite remains affected by unrelated Windows permissions/path and missing Lark CLI tests; GitNexus index refresh remains stale Related: bytedance#4984; #5016297602
github-actions
Bot
added
size/L
4 participants
Summary
Follow-up to #4912: #4912
This keeps portable
allowed-toolsparsing safe for Agent Skills declarations such as:Bash(...)stay in one entry.Bash(...)entries literal and inactive; this PR does not broaden them into unrestrictedbashaccess.Verification
uv run pytest tests/test_skills_parser.py tests/test_skills_validation.py tests/test_skills_installer.py tests/test_skill_review_core.py tests/test_skill_reviewer_public_skill.py tests/test_skill_tool_policy_middleware.py -q— 176 passedcompileallandgit diff --checkpassed.The full backend suite currently has an unrelated Windows permission-mode failure in
test_runtime_config_store_file_is_owner_only.